Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Webfonts] Change architecture to use Core's Dependencies API #43492

Merged
merged 65 commits into from
Dec 19, 2022

Conversation

hellofromtonya
Copy link
Contributor

@hellofromtonya hellofromtonya commented Aug 22, 2022

Closes #41481.
See PR #40556 for the Proof of Concept (PoC).
Replaces PR #42946.

What?

Changes the Webfonts API's architecture to use WordPress Core's Dependencies API.

Why?

From the issue #41481

The current implementation re-implements a lot of the concepts that are already handled by WP_Dependencies for the script and style loaders. The script loader classes already have the concepts of dependencies, enqueuing, and registering, as well as a way to "do_item()" (generate CSS or a tag with source, in this case).

Goals: reduce code, increase readability, and increase maintainability.

How?

Extending WP_Dependencies

The new architecture extends from WP_Dependencies. Liberties are taken to wrangle WP_Dependencies to fit the needs of web fonts.

  • do_items() processes each provider for each to print their CSS for their web fonts
  • when registering a font family and its variations, the font family and each variation goes through WP_Dependencies::add() where an instance of _WP_Dependency() is created and then stored in the queue
  • each variation is linked to its font family on registration by adding its handle into the font family's _WP_Dependency::$deps property
  • to enqueue a font family and all of its variations, invoke wp_enqueue_webfont( $font_family_handle )
  • printing happens automatically in the front-end and back-end

Additional technical notes

Backwards Compatibility: Deprecations

To help extenders know where to modify their plugins/themes and to give them time to make the changes, the following list of deprecated functions, functionality, and array structures will still work (for now) and will throw a deprecation notice.

Note: These unused deprecated items will be removed before backporting to Core.

Deprecated functions:

  • wp_register_webfont()
  • wp_enqueue_webfonts()
  • wp_get_webfont_providers()
  • The following public methods in WP_Webfonts:
    • WP_Webfonts::get_font_slug()
    • WP_Webfonts::init()
    • WP_Webfonts::get_registered_webfonts()
    • WP_Webfonts::get_enqueued_webfonts()
    • WP_Webfonts::get_all_webfonts()
    • WP_Webfonts::register_webfont()
    • WP_Webfonts::enqueue_webfont()

Web font array structure

When invoking wp_register_webfonts(), the structure of the web font definition changed:

deprecated structure

array(
    array( /* variation 1 */ ),
    array( /* variation 2 */ ),
)

new structure

array(
    'lato' => array( // the font family handle as a key is now required
          'lato-400-normal' => array( /* variation 1 */ ), // keyed is optional
          'lato-400-italic'    => array( /* variation 2 */ ), // keyed is optional
    ),
)

For backwards-compatibility (see note), WP_Webfonts::migrate_deprecated_structure() migrates the deprecated web font array structure into the new API structure. It also throws a deprecation notice to alert developers what needs to change.

Where do deprecations now live?

This PR adds a new file called webfonts-deprecations.php, which contains the deprecated global functions.

To handle the WP_Webfonts public methods BC and deprecations:

  • A new file is included for WP_Web_Fonts, which is the new API class.
  • WP_Webfonts class becomes the BC layer between as it only includes the deprecated methods.
  • To wire everything together, the inheritance is follows:
    • WP_Web_Fonts extends from WP_Webfonts extends from WP_Dependencies

Before Backporting to Core

The API has a BC layer in it to (a) give extenders time to upgrade their code and (b) keep sites from breaking if they are using the original API. This BC layer is temporary and should not be backported to Core.

BACKPORT NOTE is included in the API's code where code needs to be removed before backporting.

What to remove:

  • Remove WP_Webfonts file.
  • In WP_Web_Fonts, change the extends WP_Webfonts to extends WP_Dependencies.
  • Remove code blocks that are wrapped in BACKPORT NOTE.

Removed functions

The following methods were removed:

  • WP_Webfonts::generate_and_enqueue_styles() was a registered callback for printing
  • WP_Webfonts::generate_and_enqueue_editor_styles() was a registered callback for printing

These were registered callbacks for printing.

TODO

  • Merge the PoC
  • Get register working with theme.json
  • Get enqueue working with theme.json
  • Ensure all registrations functions go through validation first
  • Get remove working
  • Get dequeue working with theme.json
  • Test with other providers
  • Update all of the unit tests
  • Do performance benchmarking
    - [ ] Performance enhancement: Change $deps array to be keyed by the variation handles. Faster O(1) lookups vs scanning the array elements. Not worth the changes for a tiny bump.
  • Audit tests for reusable helpers and shared data sets.
  • Add tests for WP_Webfonts::dequeue().
  • Add tests for handles in enqueued queue but not in registered.
  • Reintroduce removed functions/methods but add a deprecation notice. Why? To help extenders migrate to this new API.
  • Redo the performance benchmarking

Testing Instructions

  1. Active TT3
  2. Activate the Gutenberg plugin
  3. Reset the global styles to the default by:
    • Go to the Site Editor > Global Styles "Styles Beta" UI
    • Click on the 3 vertical dots
    • Select "Reset to defaults"
    • Click on the "Save" button
  4. Examine the dynamically generated @font-face stylings in the admin area by:
    • Open Dev Tools in your favorite browser
    • Search for wp-webfonts-local (which is in the <head>)
    • Expand the <style> element to inspect the CSS
    • The CSS should be:
<style id="wp-webfonts-local">
@font-face{font-family:"Source Serif Pro";font-style:normal;font-weight:200 900;font-display:fallback;src:local("Source Serif Pro"), url('/wp-content/themes/twentytwentytwo/assets/fonts/source-serif-pro/SourceSerif4Variable-Roman.ttf.woff2') format('woff2');font-stretch:normal;}
@font-face{font-family:"Source Serif Pro";font-style:italic;font-weight:200 900;font-display:fallback;src:local("Source Serif Pro"), url('/wp-content/themes/twentytwentytwo/assets/fonts/source-serif-pro/SourceSerif4Variable-Italic.ttf.woff2') format('woff2');font-stretch:normal;}
@font-face{font-family:"DM Sans";font-style:normal;font-weight:400;font-display:fallback;src:local("DM Sans"), url('/wp-content/themes/twentytwentytwo/assets/fonts/dm-sans/DMSans-Regular.ttf') format('truetype');}
@font-face{font-family:"DM Sans";font-style:italic;font-weight:400;font-display:fallback;src:local("DM Sans"), url('/wp-content/themes/twentytwentytwo/assets/fonts/dm-sans/DMSans-Italic.ttf') format('truetype');}
@font-face{font-family:"DM Sans";font-style:normal;font-weight:700;font-display:fallback;src:local("DM Sans"), url('/wp-content/themes/twentytwentytwo/assets/fonts/dm-sans/DMSans-Bold.ttf') format('truetype');}
@font-face{font-family:"DM Sans";font-style:italic;font-weight:700;font-display:fallback;src:local("DM Sans"), url('/wp-content/themes/twentytwentytwo/assets/fonts/dm-sans/DMSans-BoldItalic.ttf') format('truetype');}
@font-face{font-family:"IBM Plex Sans";font-style:normal;font-weight:400;font-display:fallback;src:local("IBM Plex Sans"), url('/wp-content/themes/twentytwentytwo/assets/fonts/ibm-plex/IBMPlexSans-Light.woff2') format('woff2');font-stretch:normal;}
@font-face{font-family:"IBM Plex Sans";font-style:italic;font-weight:400;font-display:fallback;src:local("IBM Plex Sans"), url('/wp-content/themes/twentytwentytwo/assets/fonts/ibm-plex/IBMPlexSans-LightItalic.woff2') format('woff2');font-stretch:normal;}
@font-face{font-family:"IBM Plex Sans";font-style:normal;font-weight:200;font-display:fallback;src:local("IBM Plex Sans"), url('/wp-content/themes/twentytwentytwo/assets/fonts/ibm-plex/IBMPlexSans-ExtraLight.woff2') format('woff2');font-stretch:normal;}
@font-face{font-family:"IBM Plex Sans";font-style:italic;font-weight:200;font-display:fallback;src:local("IBM Plex Sans"), url('/wp-content/themes/twentytwentytwo/assets/fonts/ibm-plex/IBMPlexSans-ExtraLightItalic.woff2') format('woff2');font-stretch:normal;}
@font-face{font-family:"IBM Plex Mono";font-style:normal;font-weight:400;font-display:fallback;src:local("IBM Plex Mono"), url('/wp-content/themes/twentytwentytwo/assets/fonts/ibm-plex/IBMPlexMono-Text.woff2') format('woff2');font-stretch:normal;}
@font-face{font-family:"IBM Plex Mono";font-style:italic;font-weight:400;font-display:fallback;src:local("IBM Plex Mono"), url('/wp-content/themes/twentytwentytwo/assets/fonts/ibm-plex/IBMPlexMono-TextItalic.woff2') format('woff2');font-stretch:normal;}
@font-face{font-family:"IBM Plex Mono";font-style:normal;font-weight:700;font-display:fallback;src:local("IBM Plex Mono"), url('/wp-content/themes/twentytwentytwo/assets/fonts/ibm-plex/IBMPlexMono-Bold.woff2') format('woff2');font-stretch:normal;}
@font-face{font-family:"IBM Plex Mono";font-style:italic;font-weight:700;font-display:fallback;src:local("IBM Plex Mono"), url('/wp-content/themes/twentytwentytwo/assets/fonts/ibm-plex/IBMPlexSans-BoldItalic.woff2') format('woff2');font-stretch:normal;}
@font-face{font-family:Inter;font-style:normal;font-weight:200 900;font-display:fallback;src:local(Inter), url('/wp-content/themes/twentytwentytwo/assets/fonts/inter/Inter.ttf') format('truetype');font-stretch:normal;}
</style>
  1. Examine the front-end by:
    • Go to the front-end of the website
    • Open Dev Tools in your favorite browser
    • Search for wp-webfonts-local
    • Expand the <style> element to inspect the CSS
    • The CSS should be the same as above.
  2. Change to the Blue style variation and modify the "text" typography by:
    • Go back to the Global Styles UI in the admin
    • Select "Browser styles"
    • Select the "blue" variation
    • Select "Browse styles" to go back
    • Select "Typography"
    • Select "Text"
    • Change the size to "4"
    • Change the "Appearance" to "Semi Bold Italic"
    • Inspect the CSS as you did in Step 3. It should be the same.
  3. Examine the front-end by:
    • Refresh (may have to refresh multiple times until the blue styling renders)
    • Search for wp-webfonts-local
    • Expand the <style> element to inspect the CSS and verify the CSS.

Testing a plugin

This gist is a plugin that uses Jetpack's Google Font Provider package, which is using the original (deprecated) API functionality and web fonts array structure. So it should continue to work and throw deprecation notices.

Instructions of how to install and test are in the plugin's README.md file, which you can get here.

Note: This plugin has a Google Fonts Provider which means it will print <style id="wp-webfonts-jetpack-google-fonts">..</style>into the`.

@hellofromtonya hellofromtonya added [Priority] High Used to indicate top priority items that need quick attention [Feature] Webfonts labels Aug 22, 2022
@hellofromtonya hellofromtonya self-assigned this Aug 22, 2022
@hellofromtonya hellofromtonya force-pushed the try/rearchitect-webfonts-api branch from f91d4bb to 7e3e88d Compare August 23, 2022 20:54
@hellofromtonya hellofromtonya force-pushed the try/rearchitect-webfonts-api branch 2 times, most recently from 35a2b12 to 9fe815f Compare August 24, 2022 18:16
@hellofromtonya hellofromtonya marked this pull request as ready for review August 24, 2022 18:16
@hellofromtonya
Copy link
Contributor Author

@aristath and @desrosj the PR is (finally) ready for code review and testing. Feedback is encouraged and appreciated.

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big PR... 😅
I didn't manage to do a full review today, but posted some comments and will continue with the review and testing tomorrow 👍

lib/experimental/class-wp-webfonts-provider-local.php Outdated Show resolved Hide resolved
Comment on lines 126 to 130
$css = sprintf(
"<style id='wp-webfonts-local'%s>\n%s\n</style>\n",
$this->type_attr,
$css
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this <style> element be compiled here? Conceptually it would make more sense to me to get the actual CSS here - just like before, and then compile the <style> element in the WP_Webfonts_Provider::print_styles method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that. But each Provider will have its own HTML tag needs. For example, a font foundry provider may choose to build a preconnect <link> instead of dynamically generating the @font-face styles.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, for resource hints like preconnect, providers should be able to use wp_resource_hints
Though in practice, I expect all 3rd-party providers will extend the Local provider and download font-files to be hosted locally because of all the court decisions etc lately regarding font-services & GDPR

If we print the <style> element here, then perhaps we should rename the function? If it prints stuff, then the_style would make more sense than get_css

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe lean towards being verbose. 🤔

get_css() renamed to get_font_face_css(). Return only the '@font-face{' . $this->build_font_face_css( $webfont ) . '}'; in get_font_face_css(). Then provide WP_Webfonts_Provider_Local::get_style_css, which returns the final <style>.

Otherwise, leave it alone and rename get_css to get_styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep the provider's get_css() the same, dec6a3e and d4c01b3 do the following:

  • Removes the <style> element from WP_Webfonts_Provider_Local::get_css().
  • Adds WP_Webfonts_Provider::get_style_element() which gets the <style> element.
  • WP_Webfonts_Provider::print_styles() then wraps the CSS from get_css() with the <style> element.

If a Provider wants something different from <style> or its implementation, their Provider can overload the WP_Webfonts_Provider::get_style_element() to return what their needs require.

@aristath What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though in practice, I expect all 3rd-party providers will extend the Local provider and download font-files to be hosted locally because of all the court decisions etc lately regarding font-services & GDPR

While extenders have that option, some will extend from the abstract class. For example, Jetpack extends WP_Webfonts_Provider for its Google Font Provider https://github.com/Automattic/jetpack/blob/b313bd253afdf919adf7a71a866d0ab85b3a7bda/projects/packages/google-fonts-provider/src/class-google-fonts-provider.php#L18.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 👍

lib/experimental/class-wp-webfonts-utils.php Show resolved Hide resolved
lib/experimental/class-wp-webfonts-utils.php Show resolved Hide resolved
Comment on lines 96 to 112
private static function search_for_font_family( array $haystack ) {
if ( array_key_exists( 'fontFamily', $haystack ) ) {
$key = 'fontFamily';
} elseif ( array_key_exists( 'font-family', $haystack ) ) {
$key = 'font-family';
} else {
trigger_error( 'Font family not found.' );
return null;
}

if ( static::is_defined( $haystack[ $key ] ) ) {
return $haystack[ $key ];
}

trigger_error( 'Font family not defined in the variation.' );
return null;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we not rewrite this a bit to make it simpler/easier?

Suggested change
private static function search_for_font_family( array $haystack ) {
if ( array_key_exists( 'fontFamily', $haystack ) ) {
$key = 'fontFamily';
} elseif ( array_key_exists( 'font-family', $haystack ) ) {
$key = 'font-family';
} else {
trigger_error( 'Font family not found.' );
return null;
}
if ( static::is_defined( $haystack[ $key ] ) ) {
return $haystack[ $key ];
}
trigger_error( 'Font family not defined in the variation.' );
return null;
}
private static function search_for_font_family( array $haystack ) {
if ( array_key_exists( 'fontFamily', $haystack ) && static::is_defined( $haystack['fontFamily'] ) ) {
return $haystack['fontFamily'];
}
if ( array_key_exists( 'font-family', $haystack ) && static::is_defined( $haystack['font-family'] ) ) {
return $haystack['font-family'];
}
trigger_error( 'Font family not found.' );
return null;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm then it repeats the static::is_defined(). But I do agree that the PHP notices need work.

  • The first one is meant to inform the developer that the property is not defined in the variation.
  • The second one is meant to inform the developer that the font-family's value has to be a non-empty string.

2 different notifications. Both need work 🤣

Hey @aristath do you think a developer needs this level of detail?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that 2 notices here may be overkill... 😅
We can definitely simplify it and use a single notification if undefined or empty.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I say leave it as is. I see both notices being useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this? Leave it as is and then get feedback from developers if the extra informational notice is helpful or not. If not, then combine into 1 notice. Thoughts?

lib/experimental/class-wp-webfonts.php Outdated Show resolved Hide resolved
lib/experimental/class-wp-webfonts.php Outdated Show resolved Hide resolved
lib/experimental/class-wp-webfonts.php Outdated Show resolved Hide resolved
lib/experimental/class-wp-webfonts.php Outdated Show resolved Hide resolved
lib/experimental/class-wp-webfonts.php Outdated Show resolved Hide resolved
@hellofromtonya
Copy link
Contributor Author

hellofromtonya commented Sep 1, 2022

Benchmark: Average Processing Time

  • Tasks: register and enqueue (same code as at the bottom of gutenberg_register_webfonts_from_theme_json()).
  • Web fonts data: Uses the data from gutenberg_register_webfonts_from_theme_json().
  • Number of cycles: 100,000
  • How did I do it? See the instructions and code here

Ran on 17 Aug 2002

Commits used:

Results:

Architecture Average milliseconds (ms) for 100k cycles
Current 0.0207
New (PR #42946) 0.0267

Ran on 1 Sep 2002

Commits used:

Results:

Architecture Average milliseconds (ms) for 100k cycles
Current 0.0207
New (this PR) 0.0218

@colorful-tones
Copy link
Member

I plan on doing some testing for this on Friday.

Copy link
Member

@colorful-tones colorful-tones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of my stuff was nit picking. Overall, this looks like a great improvement. I ❤️ the test coverage.

Comment on lines 126 to 130
$css = sprintf(
"<style id='wp-webfonts-local'%s>\n%s\n</style>\n",
$this->type_attr,
$css
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe lean towards being verbose. 🤔

get_css() renamed to get_font_face_css(). Return only the '@font-face{' . $this->build_font_face_css( $webfont ) . '}'; in get_font_face_css(). Then provide WP_Webfonts_Provider_Local::get_style_css, which returns the final <style>.

Otherwise, leave it alone and rename get_css to get_styles.

lib/experimental/class-wp-webfonts-utils.php Show resolved Hide resolved
Comment on lines 96 to 112
private static function search_for_font_family( array $haystack ) {
if ( array_key_exists( 'fontFamily', $haystack ) ) {
$key = 'fontFamily';
} elseif ( array_key_exists( 'font-family', $haystack ) ) {
$key = 'font-family';
} else {
trigger_error( 'Font family not found.' );
return null;
}

if ( static::is_defined( $haystack[ $key ] ) ) {
return $haystack[ $key ];
}

trigger_error( 'Font family not defined in the variation.' );
return null;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I say leave it as is. I see both notices being useful.

lib/experimental/class-wp-webfonts.php Outdated Show resolved Hide resolved
lib/experimental/class-wp-webfonts.php Outdated Show resolved Hide resolved
lib/experimental/webfonts.php Outdated Show resolved Hide resolved
lib/experimental/webfonts.php Outdated Show resolved Hide resolved
lib/experimental/webfonts.php Outdated Show resolved Hide resolved
lib/experimental/webfonts.php Outdated Show resolved Hide resolved
@colorful-tones
Copy link
Member

@hellofromtonya can we update the Testing Instructions to point to Twenty Twenty-Two, please? I believe that is what we're all testing against right now to see if there are any regressions. Although, I'm sure it would be ideal to get testing against other themes that may already be leveraging some of the Webfonts APIs.

This is some monumental work here and really solid stuff. 👏

@aristath aristath force-pushed the try/rearchitect-webfonts-api branch from 6dd3d88 to e3f4d4e Compare October 3, 2022 08:42
@aristath
Copy link
Member

aristath commented Oct 3, 2022

Rebased the PR and ported a change from trunk 👍

@hellofromtonya
Copy link
Contributor Author

hellofromtonya commented Dec 13, 2022

In working through the changes needed to get the API working with Global Styles, I think the new architectural design needs more work:

  • The global functions should be minimalistic to avoid duplication of checks/guarding while abstraction of know-how should be within the class itself or its utils.
  • Migration helpers between the original and new API is too spread out.
    * Needs a centralized way to encapsulate.
    * With a centralized approach, it'll make it easier to know what code needs removing before backporting to Core.
  • Original vs new API: vastly different in publicly exposed data structures and functionality. Is there a way to keep it working without breaking sites during an interim period for extenders to upgrade?

I have ideas for each of the above. Will work on today and then push new commits for testing and discussing.

TODO:

  • Centralize deprecations
  • Minimize global functions > shift work into the WP_Web_Fonts_API
  • Verify registration works for plugins with deprecated webfonts structure and new structure (grouped by font-family)
  • Verify enqueue works for plugins

hellofromtonya and others added 7 commits December 13, 2022 10:03
To better encapsulate the deprecated WP_Webfonts methods:

* Rename the new API to `WP_Web_Fonts`
* Restore the original `WP_Webfonts` class
* Retain only the deprecated `WP_Webfonts` methods
* Reorganize the inheritance to:
WP_Dependencies > WP_Webfonts > WP_Web_Fonts
* Deprecated wp_register_webfont() in favor to use wp_register_webfont_variation()
* Centralized font-family and variations in wp_register_webfonts()
* Move deprecated global functions to a new deprecation file
* Moved the font-family name to handle conversion into WP_Web_Fonts::add_font_family()
Why?
Primary reason is to reduce the amount of code that
Core will need to maintain. If not adding value, then
remove it.

1. Each is a wrapper for `wp_webfonts()->method_name()`.
2. There's no extra context or meaning to providing these wrappers.
3. Each is likely not needed by the majority of extenders.

For those extenders who need access to this functionality,
they can use the `WP_Web_Fonts` methods instead:

* Deprecated wp_get_webfont_providers()
Instead use wp_webfonts()->get_providers()

* Removed wp_register_font_family()
Instead pass an array keyed by the font-family with an empty array
to wp_register_webfonts().
For example, wp_register_webfonts( array( 'Lato' => array() ) );

* Removed wp_register_webfont_variation()
Instead use wp_register_webfonts(). Why? To ensure the font-family
is also registered. A variation needs to have a font-family.
lib/load.php Outdated Show resolved Hide resolved
@hellofromtonya
Copy link
Contributor Author

hellofromtonya commented Dec 15, 2022

Test Report

Test Env

  • OS: macOS
  • Browser: Firefox, Edge, Chrome
  • Localhost: wp-env
  • Theme: TT3
  • Plugins: Gutenberg + Tester plugin

Test Results

  • Playfair Display Google Font (from the tester plugin):
    • Selectable in the Global Styles > Typography picker ✅
    • Visually displays in Site Editor ✅
    • Visually displays on the frontend ✅
  • Merriweather Google Font (from the tester plugin):
    • Selectable in the Global Styles > Typography picker ✅
    • Visually displays in Site Editor ✅
    • Visually displays on the frontend ✅
  • TT3's local fonts:
    • Selectable in the Global Styles > Typography picker ✅
    • Visually displays in Site Editor ✅
    • Visually displays on the frontend ✅

test1a

  • Deprecation notices (generated from the tester plugin using deprecated functionality): ✅
Deprecated: Function WP_Webfonts::get_font_slug is deprecated since version X.X.X! Use Use WP_Webfonts_Utils::convert_font_family_into_handle() to convert the font-family name into a handle instead. in /var/www/src/wp-includes/functions.php on line 5386
Deprecated: Function WP_Webfonts::get_registered_webfonts is deprecated since version X.X.X! Use wp_webfonts()->get_registered() instead. in /var/www/src/wp-includes/functions.php on line 5386
Deprecated: Function wp_enqueue_webfont is deprecated since version X.X.X! Use wp_enqueue_webfonts() instead. in /var/www/src/wp-includes/functions.php on line 5386

test2

Using the plugin's deprecated webfonts array structure

Adding ?webfonts_deprecated=1 to the URL:

  • Same results as above (meaning the API BC layer is working)
  • Deprecation notices? ✅
    all the above and this new one for the deprecated structure:
Deprecated: A deprecated web fonts array structure passed to wp_register_webfonts(). Variations must be grouped and keyed by their font family. in /var/www/src/wp-content/plugins/gutenberg/lib/experimental/class-wp-webfonts.php on line 160

test3

CSS added to the <head>

In all cases above, both expected <style> elements with @font-face CSS were generated:

<style id="wp-webfonts-local">
@font-face{font-family:"DM Sans";font-style:normal;font-weight:400;font-display:fallback;src:local("DM Sans"), url('/wp-content/themes/twentytwentythree/assets/fonts/dm-sans/DMSans-Regular.woff2') format('woff2');font-stretch:normal;}@font-face{font-family:"DM Sans";font-style:italic;font-weight:400;font-display:fallback;src:local("DM Sans"), url('/wp-content/themes/twentytwentythree/assets/fonts/dm-sans/DMSans-Regular-Italic.woff2') format('woff2');font-stretch:normal;}@font-face{font-family:"DM Sans";font-style:normal;font-weight:700;font-display:fallback;src:local("DM Sans"), url('/wp-content/themes/twentytwentythree/assets/fonts/dm-sans/DMSans-Bold.woff2') format('woff2');font-stretch:normal;}@font-face{font-family:"DM Sans";font-style:italic;font-weight:700;font-display:fallback;src:local("DM Sans"), url('/wp-content/themes/twentytwentythree/assets/fonts/dm-sans/DMSans-Bold-Italic.woff2') format('woff2');font-stretch:normal;}@font-face{font-family:"IBM Plex Mono";font-style:normal;font-weight:300;font-display:block;src:local("IBM Plex Mono"), url('/wp-content/themes/twentytwentythree/assets/fonts/ibm-plex-mono/IBMPlexMono-Light.woff2') format('woff2');font-stretch:normal;}@font-face{font-family:"IBM Plex Mono";font-style:normal;font-weight:400;font-display:block;src:local("IBM Plex Mono"), url('/wp-content/themes/twentytwentythree/assets/fonts/ibm-plex-mono/IBMPlexMono-Regular.woff2') format('woff2');font-stretch:normal;}@font-face{font-family:"IBM Plex Mono";font-style:italic;font-weight:400;font-display:block;src:local("IBM Plex Mono"), url('/wp-content/themes/twentytwentythree/assets/fonts/ibm-plex-mono/IBMPlexMono-Italic.woff2') format('woff2');font-stretch:normal;}@font-face{font-family:"IBM Plex Mono";font-style:normal;font-weight:700;font-display:block;src:local("IBM Plex Mono"), url('/wp-content/themes/twentytwentythree/assets/fonts/ibm-plex-mono/IBMPlexMono-Bold.woff2') format('woff2');font-stretch:normal;}@font-face{font-family:Inter;font-style:normal;font-weight:200 900;font-display:fallback;src:local(Inter), url('/wp-content/themes/twentytwentythree/assets/fonts/inter/Inter-VariableFont_slnt,wght.ttf') format('truetype');font-stretch:normal;}@font-face{font-family:"Source Serif Pro";font-style:normal;font-weight:200 900;font-display:fallback;src:local("Source Serif Pro"), url('/wp-content/themes/twentytwentythree/assets/fonts/source-serif-pro/SourceSerif4Variable-Roman.ttf.woff2') format('woff2');font-stretch:normal;}@font-face{font-family:"Source Serif Pro";font-style:italic;font-weight:200 900;font-display:fallback;src:local("Source Serif Pro"), url('/wp-content/themes/twentytwentythree/assets/fonts/source-serif-pro/SourceSerif4Variable-Italic.ttf.woff2') format('woff2');font-stretch:normal;}
</style>

<style id="wp-webfonts-jetpack-google-fonts">..</style>

Summary

Works as expected 🎉

@ironprogrammer
Copy link
Contributor

Test Report

Tests worked as expected for me 👍🏻 Consistent with @hellofromtonya's results 🎉

Environment

Actual Results

  • TT3 Default Styles Testing

    • #wp-webfonts-local @font-faces in <head> match between BE and FE.
    • #wp-webfonts-local styles match after switching to "Electric" (blue) style variation and modifying typography settings.
  • Web Fonts Tester

    • #wp-webfonts-local default theme @font-faces remain unchanged.
    • ✅ Google web fonts added through plugin, e.g. Lato and Arvo, display as expected on FE.
    • ✅ New #wp-webfonts-jetpack-google-fonts style element in <head> contains @font-faces for both Arvo and Lato.
    • ✅ Changing a web font, e.g. Arvo to Merriweather, updates as expected on FE.
    • ✅ After changing a web font, the #wp-webfonts-jetpack-google-fonts style element contains both Merriweather and Lato (Arvo was removed, as expected).
    • ✅ Expected deprecation notices from editor/BE:
      PHP Deprecated:  Function WP_Webfonts::get_font_slug is <strong>deprecated</strong> since version X.X.X! font-family name into a handle instead. in ... on line 5386
      PHP Deprecated:  Function WP_Webfonts::get_registered_webfonts is <strong>deprecated</strong> since version ) instead. in .../wp-includes/functions.php on line 5386
    • ✅ Expected additional deprecation notice from FE:
      PHP Deprecated:  Function wp_enqueue_webfont is <strong>deprecated</strong> since version X.X.X! Use wp_enqueue_webfonts() instead. in .../wp-includes/functions.php on line 5386
  • Web Fonts Tester (with ?webfonts_deprecated=1 querystring)

    • ✅ Same results as above, as expected.
    • ✅ Expected additional deprecation notice logged from BE and FE:
      PHP Deprecated:  A deprecated web fonts array structure passed to wp_register_webfonts(). Variations must be grouped and keyed by their font family. in .../gutenberg/lib/experimental/class-wp-webfonts.php on line 160

(No screenshots, as I have nothing to add to what was provided above.)

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a HUGE improvement!

Tested the implementation and everything works as expected.
The code looks good, and I couldn't find anything wrong.
There are some things we can tweak, but they can be done in followup PRs once this is merged 👍

@aristath
Copy link
Member

aristath commented Dec 19, 2022

I see the e2e/Playwright tests are stuck here... Probably unrelated to the PR itself. I merged trunk in this branch to trigger the tests again. Hopefully, that will make tests run properly this time so we can merge the PR. If they get stuck again, I think we can still go ahead and merge it since it's most likely a problem with the CI. 👍

Update: All tests now pass.

@hellofromtonya hellofromtonya merged commit 87c738e into trunk Dec 19, 2022
@hellofromtonya hellofromtonya deleted the try/rearchitect-webfonts-api branch December 19, 2022 11:53
@github-actions github-actions bot added this to the Gutenberg 14.9 milestone Dec 19, 2022
@hellofromtonya
Copy link
Contributor Author

Thank you everyone for contributing to this architectural redesign!!! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Priority] High Used to indicate top priority items that need quick attention
Projects
None yet
5 participants